filter_kubernetes: use service account issuer to detect EKS env #11002
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced ConfigMap-based EKS detection with serviceaccount JWT issuer inspection and removed the exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FLB as Fluent Bit
participant K8S as kubernetes_aws.c
participant FS as Filesystem
Note right of K8S #F0F4FF: determine_platform(ctx)
K8S->>FS: read serviceaccount token file
alt read fails
K8S-->>FLB: return -1
else token read
K8S->>K8S: split JWT (header.payload.signature)
alt malformed JWT
K8S-->>FLB: return -1
else JWT ok
K8S->>K8S: base64-decode payload
alt decode fails
K8S-->>FLB: return -1
else decoded payload
K8S->>K8S: extract "iss" value
alt issuer matches oidc.eks.*.amazonaws.com/id/...
Note right of K8S #DFF2E1: EKS detected
K8S-->>FLB: return 1
else no match
K8S-->>FLB: return -1
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/filter_kubernetes/kubernetes_aws.c (1)
297-300: Validate token_size after reading the Kubernetes token
Token constant is defined (kube_meta.h:55). Add a check thattoken_size > 0andtoken_size <= FLB_KUBE_TOKEN_BUF_SIZEbefore using the buffer to guard against empty or overly large token reads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_kubernetes/kubernetes_aws.c (2)
src/flb_utils.c (1)
flb_utils_read_file(1937-1940)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (5)
plugins/filter_kubernetes/kube_conf.h (1)
77-80: LGTM!The updated comment accurately reflects the new platform detection approach using serviceaccount token issuer inspection. The removal of the AWS_AUTH_CONFIG_MAP macro (mentioned in the summary) aligns with the shift away from ConfigMap-based detection.
plugins/filter_kubernetes/kubernetes_aws.c (4)
26-26: LGTM!The inclusion of
flb_utils.his necessary for theflb_utils_read_filefunction used in the new platform detection logic.
302-313: LGTM!The JWT structure validation correctly checks for the expected three-part format (header.payload.signature) and properly handles cleanup on parse failures.
315-346: Locate flb_base64_decode implementation.Run:
#!/bin/bash rg -n "flb_base64_decode" -g '*.c' -g '*.h' -C3
286-386: No changes needed for determine_platform return handling.The only caller in kube_meta.c correctly treats
ret == -1as non-EKS and all other values (i.e.1) as EKS.
| /* Look for "iss" field in the JSON payload */ | ||
| issuer_start = strstr(payload, "\"iss\":"); | ||
| if (!issuer_start) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Skip to the value part */ | ||
| issuer_start = strchr(issuer_start, ':'); | ||
| if (!issuer_start) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
| return -1; | ||
| issuer_start++; | ||
|
|
||
| /* Skip whitespace and opening quote */ | ||
| while (*issuer_start == ' ' || *issuer_start == '\t') issuer_start++; | ||
| if (*issuer_start != '"') { | ||
| flb_free(payload); | ||
| return -1; | ||
| } | ||
| issuer_start++; | ||
|
|
||
| /* Find closing quote */ | ||
| issuer_end = strchr(issuer_start, '"'); | ||
| if (!issuer_end) { | ||
| flb_free(payload); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace manual JSON parsing with a proper parser.
The current string-based parsing for the "iss" field is fragile and may fail or produce false positives in the following scenarios:
- The "iss" field appears in a nested JSON object
- The issuer value contains escaped quotes
- Another field name contains "iss" as a substring (e.g., "missing")
Since the file already includes flb_jsmn.h (line 23), use the jsmn JSON parser for robust and reliable extraction of the issuer field.
Consider applying this approach:
/* Parse JSON payload using jsmn */
jsmn_parser parser;
jsmntok_t tokens[64]; /* Adjust size as needed */
int token_count;
jsmn_init(&parser);
token_count = jsmn_parse(&parser, payload, payload_len, tokens, sizeof(tokens)/sizeof(tokens[0]));
if (token_count < 0) {
flb_free(payload);
return -1;
}
/* Find and extract "iss" field value */
char *issuer = NULL;
size_t issuer_len = 0;
for (int i = 1; i < token_count; i++) {
if (tokens[i].type == JSMN_STRING &&
strncmp(payload + tokens[i].start, "iss", tokens[i].end - tokens[i].start) == 0) {
/* Next token is the value */
if (i + 1 < token_count && tokens[i + 1].type == JSMN_STRING) {
issuer = payload + tokens[i + 1].start;
issuer_len = tokens[i + 1].end - tokens[i + 1].start;
break;
}
}
}
if (!issuer) {
flb_free(payload);
return -1;
}🤖 Prompt for AI Agents
plugins/filter_kubernetes/kubernetes_aws.c lines 347-375: Replace the brittle
string-based "iss" extraction with jsmn parsing: initialize a jsmn_parser, parse
payload using payload_len into a tokens array (resize tokens if needed), check
token_count for errors and free payload on failure, then iterate tokens to find
a JSMN_STRING token whose content exactly equals "iss" (use
tokens[i].start/end), verify the next token exists and is a string, and set
issuer_start = payload + tokens[i+1].start and issuer_end = payload +
tokens[i+1].end; on any failure free payload and return -1; this handles nested
objects and escaped characters correctly.
movence
force-pushed
the
aws-eks-detection
branch
from
721ae39 to
1a27fbf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_kubernetes/kube_conf.h(1 hunks)plugins/filter_kubernetes/kubernetes_aws.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/filter_kubernetes/kubernetes_aws.c
🔇 Additional comments (1)
plugins/filter_kubernetes/kube_conf.h (1)
77-80: All references to AWS_AUTH_CONFIG_MAP have been removed
Search forAWS_AUTH_CONFIG_MAPacross.cand.hfiles returned no results, confirming safe removal.
Signed-off-by: Hyunsoo Kim <[email protected]>
movence
force-pushed
the
aws-eks-detection
branch
from
1a27fbf to
1c0104a
Compare
| * on EKS or native Kubernetes by inspecting serviceaccount token issuer | ||
| */ | ||
| #define KUBE_SYSTEM_NAMESPACE "kube-system" | ||
| #define AWS_AUTH_CONFIG_MAP "aws-auth" |
There was a problem hiding this comment.
Should we keep this as a fallback for old clusters? Or does the issuer exist on both old and new cluster so it should just be the definitive mechanism for determining platforms?
There was a problem hiding this comment.
That is a good point. We found that the EKS service account issuer was introduced starting with EKS version 1.21, which has been EOL for quite some time now. This gave us confidence to move forward with this approach without a fallback.
Signed-off-by: Hyunsoo Kim <[email protected]>
movence
force-pushed
the
aws-eks-detection
branch
from
10f6052 to
8b53611
Compare
Signed-off-by: Hyunsoo Kim <[email protected]>
|
Hey @edsiper could you check this out since we've discussed it before? |
3 participants
Background
Explored Related introduced
filter_kubernetesto add Kubernetes metadata to logs and metrics. The filter historically relied on the presence of theaws-authConfigMap to determine if the cluster is running on Amazon EKS. However, with the launch of EKS access entries, theaws-authConfigMap is no longer guaranteed to be present in EKS clusters.Problem
With the launch of EKS access entries,
aws-authConfigMap is no longer guaranteed to be present in EKS clusters. Missing ConfigMap causes two issues:- Noisy audit logs: Generates 404 not found errors in k8s audit logs when the
- Incorrect platform tagging: EKS clusters using access entries (
NAaws-authConfigMap doesn't existAPI) are falsely tagged asGenericorK8splatform instead ofAWS::EKSfor "Explore Related" functionalityEnter
[N/A]in the box, if an item is not applicable to your change.Testing
Using an EKS cluster with
authemticationModeset toAPIto preventaws-authconfigmap from being createdUse CloudWatch LogInsights to search application log entries with metadata. Note the value for
@entity.Attributes.PlatformTypeIf this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation